Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid XSS from data-verify / data-caution attributes #335

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

Crabcyborg
Copy link
Contributor

@Crabcyborg Crabcyborg commented Jan 21, 2021

Fixes a vulnerability Steph mentions in https://github.com/Strategy11/formidable-pro/issues/2827#issuecomment-762478526

This can be exploited when someone creates an anchor tag that matches our expected attribute.

Also fixes https://github.com/Strategy11/formidable-pro/issues/2846

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #335 (febd8bb) into master (1c009ca) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #335      +/-   ##
============================================
- Coverage     26.18%   26.17%   -0.01%     
  Complexity     5740     5740              
============================================
  Files            80       80              
  Lines         15747    15747              
============================================
- Hits           4123     4122       -1     
- Misses        11624    11625       +1     
Impacted Files Coverage Δ Complexity Δ
classes/helpers/FrmAppHelper.php 26.60% <0.00%> (-0.09%) 455.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c009ca...febd8bb. Read the comment docs.

@stephywells
Copy link
Contributor

This looks great! Thanks so much for solving these!

I think we're still missing part of the external image issue. If someone adds the html in an entry, and then the entry is viewed in the admin area or using a view, loading the image would be vulnerable. Did you see my note about this in the issue?

Copy link
Contributor

@stephywells stephywells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. We just need to double check the upgrade messages that include HTML. Some include images in the upgrade message. @jairoprez can you test this in the lite version?

Copy link
Contributor

@jairoprez jairoprez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephywells stephywells merged commit 8612efa into master Jan 27, 2021
@Crabcyborg Crabcyborg deleted the verify_xss_fix branch March 26, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants